Fluctuating collateral value during redemption proposal dispute period exposes the redeemer or protocol to potential gains or losses depending on the asset price movement

[c4-bot-10]

Lines of code

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

Vulnerability details

Impact

When the redeemer submits their candidate shortRecords, the collateral is calculated based on the asset price (LibOracle::getPrice) and the redemption amount:

File: RedemptionFacet.sol
056:     function proposeRedemption(
057:         address asset,
058:         MTypes.ProposalInput[] calldata proposalInput,
059:         uint88 redemptionAmount,
060:         uint88 maxRedemptionFee
061:     ) external isNotFrozen(asset) nonReentrant {
...
...
075:         p.oraclePrice = LibOracle.getPrice(p.asset);
...
...
115: 
116:             p.colRedeemed = p.oraclePrice.mulU88(p.amountProposed);
117:             if (p.colRedeemed > currentSR.collateral) {
118:                 p.colRedeemed = currentSR.collateral;
119:             }

Additionally, the timeToDispute is calculated based on the collateral ratio of the proposals added to the redeemer's slate, which can extend up to 6 hours. The issue arises during this period when the asset price changes, resulting in inconsistencies in the collateral received in RedemptionFacet::claimRedemption or RedemptionFacet::disputeRedemption. This exposes the redeemer or protocol to potential gains or losses depending on the asset price movement.

Proof of Concept

Consider the following scenario:

1. The price is 3000 USD per 1 ETH.
2. The redeemer submits some short proposals and waits for the timeToDispute to elapse. At the current price, the redeemer would receive 2 ETH collateral for redeeming 6000 USD (0.00033 * 6000 USD = 2 ETH) when the dispute period ends.
3. However, during the dispute period, the price of ETH increases to 3500 USD per 1 ETH.
4. When the timeToDispute ends, the redeemer calls RedemptionFacet::claimRedemption, receiving collateral calculated using the outdated price (2 ETH). However, since the ETH price increased, the correct collateral should be less (0.00028 * 6000 USD = 1.68 ETH).
5. Consequently, the redeemer withdraws 2 ETH with only 6000 USD, which is incorrect because the currentETH price is 3500 USD.

The provided test demonstrates that the collateral delivered in RedemptionFacet::claimRedemption remains unchanged from the initial calculation in RedemptionFacet::proposeRedemption, leading to incorrect collateral distribution if the asset price changes during the dispute period.

    // File: test/Redemption.t.sol
    function test_proposeRedemptionCollateralIsNotRecalculated() public {
        //
        address redeemer = address(1212);
        depositUsd(redeemer, DEF_REDEMPTION_AMOUNT);
        depositEth(redeemer, DEFAULT_AMOUNT);
        //
        // 1. Match orders
        fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, receiver);
        fundLimitShortOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, sender);
        fundLimitBidOpt(DEFAULT_PRICE + 2, DEFAULT_AMOUNT, receiver);
        fundLimitShortOpt(DEFAULT_PRICE + 2, DEFAULT_AMOUNT, extra);
        skip(1 days);
        //
        // 2. Price drops. Redeemer calls `proposeRedemption`
        _setETH(1000 ether);
        MTypes.ProposalInput[] memory proposalInputs = new MTypes.ProposalInput[](2);
        proposalInputs[0] = MTypes.ProposalInput({shorter: sender, shortId: C.SHORT_STARTING_ID, shortOrderId: 0});
        proposalInputs[1] = MTypes.ProposalInput({shorter: extra, shortId: C.SHORT_STARTING_ID, shortOrderId: 0});
        vm.prank(redeemer);
        diamond.proposeRedemption(asset, proposalInputs, DEFAULT_AMOUNT * 2, MAX_REDEMPTION_FEE);
        MTypes.ProposalData[] memory decodedProposalData = getSlate(redeemer);
        assertEq(decodedProposalData.length, 2);
        //
        // Time to dispute dispute has elapsed
        skip(1 days);
        //
        // 3. Price is increased. The redeem's collateral should be less because the price went up
        _setETH(2000 ether);
        //
        // 4. Claim redemption. The collateral has not changed even when the price has been increased
        uint256 redeemerBalanceBefore = diamond.getVaultUserStruct(vault, redeemer).ethEscrowed;
        vm.prank(redeemer);
        diamond.claimRedemption(asset);
        assertEq(
            redeemerBalanceBefore + decodedProposalData[0].colRedeemed + decodedProposalData[1].colRedeemed,
            diamond.getVaultUserStruct(vault, redeemer).ethEscrowed);
    }

The same can happen when there is an effective dispute because when it occurs, the collateral calculated in RedemptionFacet::proposeRedemption is returned. However, this collateral was calculated using an asset price calculated up to a maximum of 6 hours ago, which most likely means the price is outdated, thus causing the return of collateral in the dispute with a price that may very well be outdated:

File: RedemptionFacet.sol
224:     function disputeRedemption(address asset, address redeemer, uint8 incorrectIndex, address disputeShorter, uint8 disputeShortId)
225:         external
226:         isNotFrozen(asset)
227:         nonReentrant
228:     {
...
...
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
263:             for (uint256 i = incorrectIndex; i < decodedProposalData.length; i++) {
264:                 currentProposal = decodedProposalData[i];
265: 
266:                 STypes.ShortRecord storage currentSR = s.shortRecords[d.asset][currentProposal.shorter][currentProposal.shortId];
267:                 currentSR.collateral += currentProposal.colRedeemed;
268:                 currentSR.ercDebt += currentProposal.ercDebtRedeemed;
269: 
270:                 d.incorrectCollateral += currentProposal.colRedeemed;
271:                 d.incorrectErcDebt += currentProposal.ercDebtRedeemed;
272:             }
...
...

It can also lead to a lack of incentive for the redeemer since the following scenario can occur:

1. The price is at 3000 USD per 1 eth.
2. The redeemer submits some shortRecords proposals and waits until the timeToDispute period passes. The redeemer wants to redeem 6000 USD, and at the current price, they would receive 2 eth as collateral (0.00033 * 6000 USD = 2 eth) when the dispute period ends.
3. However, while the dispute period is active, the price of ETH decreases to 2500 USD per 1 eth.
4. Finally, the timeToDispute period ends, and the redeemer calls RedemptionFacet::claimRedemption, resulting in receiving 2 eth as collateral. However, in this case, since the price of the asset eth decreased, the collateral that should be delivered to the redeemer should be more; it should be (0.0004 * 6000 USD = 2.4 eth).

Therefore, there is a lack of incentives for the redeemer to use this system since they are exposed to price fluctuations, and the collateral they receive may not be adequate depending on the movement of the asset's price.

Tools used

Manual review

Recommended Mitigation Steps

The recommendation is to calculate the collateral after the dispute period has elapsed. Additionally, enable the redeemer to specify a slippage tolerance for asset price movements during the dispute period to mitigate the risk of receiving inadequate collateral due to price changes.

Assessed type

Context

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

Slippage should be inevitable in this context.

[ditto-eth]

Collateral is set aside for the redeemer at the moment of proposal, it's just not accessible until the wait period is over. Agree that it's not ideal but it is how the system was designed, yes slippage is inevitable for the sake of accurate redemptions (ie. allow space for disputes)

[c4-sponsor]

ditto-eth (sponsor) disputed

[hansfriese]

Agree with the sponsor.

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid

[0xbepresent]

Hi @hansfriese

The report specifically mentions how there can be losses either for the user or for the protocol itself due to fluctuations in ETH (which are clearly beyond the user's control). As auditors, we need to highlight such errors, and if this error was not specified as a "known error," then I don't see the reason for invalidating it.

[hansfriese]

I understand your concern but it's an intended behavior of the protocol. So the protocol uses the cached price at the time of submitting a proposal.