c4-bot-5 commented 3 months ago

Lines of code

https://github.com/code-423n4/2024-07-dittoeth/blob/ca3c5bf8e13d0df6a2c1f8a9c66ad95bbad35bce/contracts/tokens/yDUSD.sol#L84 https://github.com/code-423n4/2024-07-dittoeth/blob/ca3c5bf8e13d0df6a2c1f8a9c66ad95bbad35bce/contracts/tokens/yDUSD.sol#L36 https://github.com/code-423n4/2024-07-dittoeth/blob/ca3c5bf8e13d0df6a2c1f8a9c66ad95bbad35bce/contracts/facets/OrdersFacet.sol#L139 https://github.com/code-423n4/2024-07-dittoeth/blob/ca3c5bf8e13d0df6a2c1f8a9c66ad95bbad35bce/contracts/libraries/LibPriceDiscount.sol#L46

Vulnerability details

Impact

The function yUSD::checkDiscountWindow is designed to check if the system has had discounted sales. Additionally, the yDUSD::withdraw function allows users to withdraw dUSD that have gone through a proposed amount process and are within a 45-day withdrawal period.

The issue arises with the call to the function getInitialDiscountTime() within yUSD::checkDiscountWindow, which will always return more than one second even when an order book becomes inactive.

File: yDUSD.sol
36:     function checkDiscountWindow() internal {
37:         //Note: Will upgrade contract to diamond in the future
38:         bool WithinDiscountWindow = IDiamond(payable(diamond)).getTimeSinceDiscounted(dusd) < C.DISCOUNT_WAIT_TIME;
39:         bool isDiscounted = IDiamond(payable(diamond)).getInitialDiscountTime(dusd) > 1 seconds;
40:         if (isDiscounted || WithinDiscountWindow) revert Errors.ERC4626CannotWithdrawBeforeDiscountWindowExpires();
41:     }

An inactive order book (lacking sell/bid order matches) can create bottlenecks or delays in withdrawals since they only allow withdrawals under specific time frames. The checkDiscountWindow function will always revert due to getInitialDiscountTime not being reset during periods of inactivity in the order book.

Proof of Concept

It can be observed that the getInitialDiscountTime retrieves the value of initialDiscountTime:

File: ViewFacet.sol
489: 
490:     function getInitialDiscountTime(address asset) external view returns (uint32 initialDiscountTime) {
491:         return s.asset[asset].initialDiscountTime;
492:     }

This variable is only modified when there is activity in the order book and the LibPriceDiscount::handlePriceDiscount function is called in BidOrdersFacet#L338 and LibOrders#L659:

File: BidOrdersFacet.sol
336: 
337:         // @dev match price is based on the order that was already on orderbook
338:         LibPriceDiscount.handlePriceDiscount(asset, matchTotal.lastMatchPrice, matchTotal.fillErc);
339:         return (matchTotal.fillEth, incomingBid.ercAmount);
340:     }

File: LibOrders.sol
658:         // @dev match price is based on the order that was already on orderbook
659:         LibPriceDiscount.handlePriceDiscount(asset, matchTotal.lastMatchPrice, matchTotal.fillErc);
660:     }

If this function is not called due to inactivity in the orderBook, initialDiscountTime will always be more than a second, causing the checkDiscountWindow to revert this results in withdrawals being affected. Consider the scenario where, for example, there is a one-hour discount sales window, and then seven hours pass without any activity in the orderBook. However, initialDiscountTime will have a value greater than one second because there were penalties applied seven hours ago. Currently, these penalties are no longer being applied, so the withdrawal should be available, especially since the function itself has a period during which a withdrawal can be made; otherwise, the process must be restarted.

The following test demonstrates how the user is unable to make a withdrawal because the order book is inactive. Even after the time to make a withdrawal has passed, the user would need to call proposeWithdraw again to restart the process, however, this is not possible (step 5 revert) as the order book remains inactive.:

    // File: YDUSD.t.sol
    function test_checkDiscountRevertInactiveOrderBook() public {
        uint64 discountPenaltyFee = uint64(diamond.getAssetNormalizedStruct(asset).discountPenaltyFee);
        // User 1 despoit DUSD into vault to get yDUSD
        assertEq(token.balanceOf(_yDUSD), 0);
        vm.prank(receiver);
        rebasingToken.deposit(DEFAULT_AMOUNT, receiver);
        assertEq(token.balanceOf(_yDUSD), DEFAULT_AMOUNT);
        //
        // 1. User calls `proposeWithdraw()`
        skip(C.DISCOUNT_WAIT_TIME);
        uint88 withdrawAmount = DEFAULT_AMOUNT;
        vm.prank(receiver);
        rebasingToken.proposeWithdraw(withdrawAmount);
        //
        // 2. Create discount to generate newDebt
        uint88 newDebt = uint88(discountSetUp());
        assertEq(token.balanceOf(_yDUSD), DEFAULT_AMOUNT + newDebt);
        //
        // 3. Order book is not active for long period. User cannot withdraw
        skip(20 days);
        vm.expectRevert(Errors.ERC4626CannotWithdrawBeforeDiscountWindowExpires.selector);
        vm.prank(receiver);
        rebasingToken.withdraw(withdrawAmount, receiver, receiver);
        //
        // 4. OrderBook is still not active. Now user needs to call again the `proposeWithdraw` and wait for the propose time window
        skip(40 days);
        vm.expectRevert(Errors.ERC4626MaxWithdrawTimeHasElapsed.selector);
        vm.prank(receiver);
        rebasingToken.withdraw(withdrawAmount, receiver, receiver);
        //
        // 5. Call `proposeWithdraw` but it will be reverted
        vm.prank(receiver);
        rebasingToken.cancelWithdrawProposal();
        vm.expectRevert(Errors.ERC4626CannotWithdrawBeforeDiscountWindowExpires.selector);
        vm.prank(receiver);
        rebasingToken.proposeWithdraw(withdrawAmount);
    }

Tools used

Manual review

Recommended Mitigation Steps

It is recommended to implement a rule that allows for a window of inactivity in the order book, enabling withdrawals during this period.

Assessed type

Invalid Validation

c4-judge commented 3 months ago

hansfriese marked the issue as primary issue

ditto-eth commented 3 months ago

i believe this scenario is unlikely, but if the orderbook is truly inactive then the user can match themselves at oracle price with a small amount to update the time

hansfriese commented 3 months ago

QA is more appropriate given the unlikely scenario and the available resolution for users. I will invalidate it since there is no QA pool.