OpenEden Disclosed Report

https://github.com/OpenEdenHQ/openeden.vault.audit/tree/d18288e944df21729b18d430b2afec2da99b6287

Summary

Lets take a look at redeem control flow:

• User calls redeem and the sender/receiver KYC is validated and the redeem details is pushed to withdrawal queue
• After sometime the Operator role process the withdrawalQueue when the assets are trasnferred to the vault to be redeemed by the receivers via the processWithdrawalQueue

We notice that the processWithdrawalQueue also calls the _validateKyc function while processing each withdraw, this is to ensure that the sender or the receiver was not banned/unKYCed during the timespan between calling redeem and withdraw processing.

But this could lead to a DoS of the withdrawals for all users who are queued after the banned user. Trying to cancel the withdraw request will lead to reverts too because cancel tries to _trasnfer the shares back to the sender but it internally calls __beforeTokenTransfer which again validates the KYC status. This leaves the protocol in a complete DoS state as the queue could neither be processed nor be cancelled.

Target Code Snippets:

• https://github.com/OpenEdenHQ/openeden.vault.audit/blob/d18288e944df21729b18d430b2afec2da99b6287/contracts/OpenEdenVaultV4Impl.sol#L281

• https://github.com/OpenEdenHQ/openeden.vault.audit/blob/d18288e944df21729b18d430b2afec2da99b6287/contracts/OpenEdenVaultV4Impl.sol#L232

Details

If a user is banned after their withdrawal is queued to the withdrawalQueue, i.e. after they have executed redeem, then processing or cancelling the withdrawalQueue can lead to complete DoS of the system unless the user is unbanned which defeats the enforcement purpose of the ban and creates undesirable trust assumptions around reversibility of such actions.

The impacts are best described in the PoC.

Recommendation

Follow the pull instead of push style for withdrawal processing and cancellations, i.e., these functions should execute only the core logic and the transfers to users and kyc validations should be done in a different function which allows users to call the function and get back their assets/shares instead of the protocol trying to transfer them to the user. A possible mititgation could be:

For cancelling withdrawals

Keep a state that tracks cancelled shares and remove the _trasnfer logic to a different function which can be called by user to withdraw their cancelled withdrawal shares.

    mapping(address => uint256) public cancelledWithdrawal;

    function cancel(uint256 _len) external onlyMaintainer {
        if (_len > withdrawalQueue.length()) revert TBillInvalidInput(_len);
        controller.requireNotPausedWithdraw();
        uint256 totalShares;

        while (_len > 0) {
            bytes memory data = withdrawalQueue.popFront();

            (
                address sender,
                address receiver,
                uint256 shares,
                bytes32 prevId
            ) = _decodeData(data);

            unchecked {
                totalShares += shares;
                withdrawalInfo[receiver] -= shares;
                _len--;
                cancelledWithdrwal[sender] += shares; // <@ state to track cancelled shares
            }
            _transfer(address(this), sender, shares); // <@ remove this logic to a different function 
            emit ProcessRedeemCancel(sender, receiver, shares, prevId);
        }
        emit Cancel(_len, totalShares);
    }

    function receiveCancelledShares() external{
      uint256 shares = cancelledWithdrawal[msg.sender];
      _transfer(address(this), msg.sender, shares);
    }

For processing withdrawals

Keep a state that tracks to be withdrawn assets by the receiver and the shares to be burned and remove the KYC validation and asset withdrawal logic to a different function.

    mapping(address => uint256) assetsToWithdraw;
    mapping(address => uint256) sharesToBurn;

    function processWithdrawalQueue(uint _len) external onlyOperator {
        uint256 length = withdrawalQueue.length();
        if (length == 0 || _len > length) revert TBillInvalidInput(_len);
        if (_len == 0) _len = length;

        uint256 totalWithdrawAssets;
        uint256 totalBurnShares;
        uint256 totalFees;

        for (uint count = 0; count < _len; ) {
            bytes memory data = withdrawalQueue.front();
            (
                address sender,
                address receiver,
                uint256 shares,
                bytes32 prevId
            ) = _decodeData(data);

            _validateKyc(sender, receiver); // <@ remove the KYC Validation from here
            uint256 assets = _convertToAssets(shares);

            // 1. will not process the queue if the assets is not enough
            // 2. will process the queue by sequence, so if the first one is not enough, the rest will not be handled
            if (assets > onchainAssets()) {
                break;
            }

            // will calculate the fee based on the lastest fee rate, not the fee rate when the user redeem
            // if not enough, will revert
            (uint256 oeFee, int256 pFee, uint256 fee) = txsFee(
                ActionType.REDEEM,
                sender,
                assets
            );

            // collect the fee
            if (fee > 0) {
                SafeERC20Upgradeable.safeTransfer(
                    IERC20Upgradeable(underlying),
                    oplTreasury,
                    fee
                );
            }

            withdrawalQueue.popFront();
            uint256 trimmedAssets = assets - fee;
            unchecked {
                ++count;
                totalWithdrawAssets += trimmedAssets;
                totalBurnShares += shares;
                totalFees += fee;
                withdrawalInfo[receiver] -= shares;
                assetsToWithdraw[receiver] += trimmedAssets // <@ track assets to withdraw
                sharesToBurn[receiver] +=s hares // <@ track shares to burn
            }

            _withdraw(
                address(this),
                receiver,
                address(this),
                trimmedAssets,
                shares
            ); // <@ remove this logic to a different function 
    }

    function receiveWithdrawnAssets() external{
      uint256 trimmedAssets = assetsToWithdraw[msg.sender];
      uint256 shares = sharesToBurn[msg.sender];
      _validateKyc(sender, receiver);
      _withdraw(
          address(this),
          msg.sender,
          address(this),
          trimmedAssets,
          shares
      );
    }
    }

PoC

Step 1:

Two users Alice and Bob call redeem, their vault shares are transferred to vault and their withdraw details are pushed to withdrawalQueue [alice, bob]

Step 2:

Alice is banned due to some violations

Step 3: Now after the USDC is sent to the vault the operator calls processWithdrawalQueue which would now revert because of the _validateKyc check inside the for loop to process each user withdrawals.

Note: If there were users before Alice in the queue, they are not impacted because the _len could be used to process the users before Alice. But no users after Alice can get their withdrawals processed as one banned user leads to reverting the whole withdraw queue.

Step 4: Maintainer calls cancel to cancel Alices's withdrawal as a possible mitigation, but this reverts too since trying to cancel the withdrawal internally calls _transfer to send the shares back to Alice, which internally calls __beforeTokenTransfer which again does a _validateKYC

The only available workaround becomes unbanning Alice to allow the system to proceed, which is counterintuitive to the goals of banning and could compromise the integrity of the KYC enforcement mechanism.

Impact

• processing withdrawals is DoSed
• cancel is DoSed
• The sender shares are stuck in the vault contract without any way to rescue them