c4-bot-9 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/RedemptionFacet.sol#L56-L177 https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/RedemptionFacet.sol#L31-L43 https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/RedemptionFacet.sol#L87 https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/RedemptionFacet.sol#L31-L43 https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/RedemptionFacet.sol#L116-L125

Vulnerability details

Summary & Impact

proposeRedemption function allows users to propose a set of ShortRecords for redemption. It takes the following parameters:

asset: The market that will be impacted.
proposalInput: Array of data pertaining to the ShortRecord candidates.
redemptionAmount: Total amount of ercDebt requested to be redeemed.
maxRedemptionFee: Maximum fee that the redeemer is willing to pay.

Then perform the following steps:

Validates the input parameters and checks the redeemer's escrowed balance.
Iterates through the proposalInput array and validates each ShortRecord candidate.
Calculates the total collateral and debt to be redeemed based on the valid ShortRecords.
Stores the redemption proposal data using SSTORE2.
Updates the system's debt and the redeemer's escrowed balance.

RedemptionFacet.sol::proposeRedemption

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

There's Insufficient validation of the proposed ShortRecords in the proposeRedemption. The function relies on the validRedemptionSR function to validate each ShortRecord candidate. But, the validation checks in validRedemptionSR may not cover all necessary conditions to ensure the integrity of the proposed ShortRecords.

For example, the validRedemptionSR function checks if the ShortRecord's status is not SR.Closed and if the ercDebt is greater than or equal to minShortErc. However, it does not verify other important aspects, such as the ShortRecord's collateral ratio or the validity of the associated short order.

RedemptionFacet.sol::validRedemptionSR

function validRedemptionSR(STypes.ShortRecord storage shortRecord, address proposer, address shorter, uint256 minShortErc)
internal
view
returns (bool)
{
// @dev Matches check in onlyValidShortRecord but with a more restrictive ercDebt condition
// @dev Proposer can't redeem on self
if (shortRecord.status == SR.Closed || shortRecord.ercDebt < minShortErc || proposer == shorter) {
    return false;
} else {
    return true;
}
}

The insufficient validation checks in the validRedemptionSR function, an attacker can potentially propose ShortRecords that are invalid or manipulated. For example, an attacker could propose ShortRecords with extremely low collateral ratios or ShortRecords associated with invalid short orders. Since these conditions are not properly validated, the function may accept these invalid ShortRecords as part of the redemption proposal.

Impact

Incorrect redemption amounts could be calculated based on the invalid or manipulated ShortRecords proposed by an attacker. This could lead to the redemption of unintended collateral and debt amounts, potentially draining funds from the system or disrupting the protocol's stability. Attackers may exploit this vulnerability to gain an unfair advantage or manipulate the redemption process for their benefit.

Proof of Concept

Step 1: Attacker identifies a ShortRecord with a low collateral ratio. Let's assume there exists a ShortRecord with the following properties:

shortRecord.status = SR.FullyFilled
shortRecord.ercDebt = 1000 DUSD
shortRecord.collateral = 1100 DETH
shortRecord.shortOrderId = 0 (no associated short order)

The collateral ratio of this ShortRecord is approximately 1.1 (1100 DETH / 1000 DUSD), which is considered low.

Step 2: Attacker prepares the proposalInput array. The attacker creates a proposalInput array that includes the identified ShortRecord:

MTypes.ProposalInput[] memory proposalInput = new MTypes.ProposalInput[](1);
proposalInput[0] = MTypes.ProposalInput({
    shorter: shorterAddress,
    shortId: shortRecordId
});

Step 3: Attacker calls the proposeRedemption function. The attacker calls the proposeRedemption function with the prepared proposalInput array:

proposeRedemption(
    asset,
    proposalInput,
    1000, // redemptionAmount
    10    // maxRedemptionFee
);

Step 4: The proposeRedemption function processes the proposed ShortRecord. Inside the proposeRedemption function, the validRedemptionSR function is called to validate the proposed ShortRecord: #L87

if (!validRedemptionSR(currentSR, msg.sender, p.shorter, minShortErc)) continue;

However, the validRedemptionSR function only checks for the ShortRecord's status, ercDebt, and the proposer's address: https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/RedemptionFacet.sol#L31-L43

function validRedemptionSR(STypes.ShortRecord storage shortRecord, address proposer, address shorter, uint256 minShortErc)
    internal
    view
    returns (bool)
{
    if (shortRecord.status == SR.Closed || shortRecord.ercDebt < minShortErc || proposer == shorter) {
        return false;
    } else {
        return true;
    }
}

Since the attacker's proposed ShortRecord passes these checks, it is considered valid and included in the redemption proposal.

Step 5: The redemption proposal is processed. The proposeRedemption function continues to process the redemption proposal, calculating the total collateral and debt to be redeemed based on the proposed ShortRecord: RedemptionFacet.sol#116-L125

p.colRedeemed = p.oraclePrice.mulU88(p.amountProposed);
if (p.colRedeemed > currentSR.collateral) {
    p.colRedeemed = currentSR.collateral;
}

currentSR.collateral -= p.colRedeemed;
currentSR.ercDebt -= p.amountProposed;

p.totalAmountProposed += p.amountProposed;
p.totalColRedeemed += p.colRedeemed;

Step 6: Impact on the protocol and users. By successfully proposing a ShortRecord with a low collateral ratio, the attacker can potentially redeem more collateral than what is fair based on the ShortRecord's debt. In this example, the attacker can redeem 1100 DETH by proposing to redeem 1000 DUSD, effectively gaining an extra 100 DETH.

This exploitation can lead to the drainage of funds from the protocol and disrupt the overall stability and solvency of the system. If multiple attackers perform similar exploits or if the attacker proposes a large number of such ShortRecords, the impact can be significant.

Furthermore, the attack can be amplified if the attacker proposes ShortRecords associated with invalid short orders, as the validRedemptionSR function does not validate the integrity of the associated short order.

Tools Used

Vs

Recommended Mitigation Steps

The validRedemptionSR should be enhanced to perform more comprehensive validation checks on the proposed ShortRecords.

RedemptionFacet.sol::

function validRedemptionSR(STypes.ShortRecord storage shortRecord, address proposer, address shorter, uint256 minShortErc)
    internal
    view
    returns (bool)
{
    if (shortRecord.status == SR.Closed || 
        shortRecord.ercDebt < minShortErc || 
        proposer == shorter ||
+       shortRecord.collateralRatio < minCollateralRatio ||
+       !isValidShortOrder(shortRecord.shortOrderId)) {
        return false;
    } else {
        return true;
    }
}

I've introduced additional checks to verify the ShortRecord's collateral ratio and the validity of the associated short order. The minCollateralRatio can be set to a reasonable value to ensure that only ShortRecords with sufficient collateral are considered valid for redemption. The isValidShortOrder function (to be implemented separately) should check the validity of the short order associated with the ShortRecord.

Assessed type

Other

c4-pre-sort commented 5 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 5 months ago

raymondfam marked the issue as duplicate of #171

raymondfam commented 5 months ago

Paralleling #171 and more prone to user's fault.

c4-judge commented 4 months ago

hansfriese marked the issue as unsatisfactory: Invalid

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

Potential redemption of unintended collateral, disrupting protocol stability, risking financial loss. #214