Assessment reports>WOOFI Stake>Medium findings>Possible DOS
Category: Coding Mistakes

Possible DOS

Medium Severity
Medium Impact
Medium Likelihood

Description

The requestWithdrawShares is part of the withdrawal mechanism for the users of the SuperChargerVault:

function _requestWithdrawShares(uint256 shares) private {
    require(shares > 0, "WooSuperChargerVault: !amount");
    require(!isSettling, "WooSuperChargerVault: CANNOT_WITHDRAW_IN_SETTLING");
    require(requestUsers.length() <= maxWithdrawCount, "WooSuperChargerVault: MAX_WITHDRAW_COUNT");

    address owner = msg.sender;

    lendingManager.accureInterest();
    uint256 amount = _assets(shares);
    TransferHelper.safeTransferFrom(address(this), owner, address(this), shares);

    requestedWithdrawShares[owner] = requestedWithdrawShares[owner] + shares;
    requestedTotalShares = requestedTotalShares + shares;
    requestUsers.add(owner);

    emit RequestWithdraw(owner, amount, shares);
}

This function marks the amount of shares that the user wants to withdraw and adds the user to the list of users that have requested a withdrawal.

function endWeeklySettle() public onlyAdmin {
    require(isSettling, "!SETTLING");
    require(weeklyNeededAmountForWithdraw() == 0, "WEEKLY_REPAY_NOT_CLEARED");

    // NOTE: Do need accureInterest here, since it's already been updated in `startWeeklySettle`
    uint256 sharePrice = getPricePerFullShare();

    isSettling = false;
    uint256 totalWithdrawAmount = requestedTotalAmount();

    if (totalWithdrawAmount != 0) {
        uint256 shares = _sharesUp(totalWithdrawAmount, reserveVault.getPricePerFullShare());
        reserveVault.withdraw(shares);

        if (want == weth) {
            IWETH(weth).deposit{value: totalWithdrawAmount}();
        }
        require(available() >= totalWithdrawAmount);

        uint256 length = requestUsers.length();

        // @audit-issue DOS here if attacker performs lots of requests.
        for (uint256 i = 0; i < length; i++) {
            address user = requestUsers.at(0);

            withdrawManager.addWithdrawAmount(user, (requestedWithdrawShares[user] * sharePrice) / 1e18);

            requestedWithdrawShares[user] = 0;
            requestUsers.remove(user);
        }

        _burn(address(this), requestedTotalShares);
        requestedTotalShares = 0;

        TransferHelper.safeTransfer(want, address(withdrawManager), totalWithdrawAmount);
    }

    instantWithdrawnAmount = 0;

    lendingManager.accureInterest();
    uint256 totalBalance = balance();
    instantWithdrawCap = totalBalance / 10;

    emit WeeklySettleEnded(msg.sender, totalBalance, lendingBalance(), reserveBalance());
}

The endWeeklySettle function is called by the admin to end the weekly settlement. It calculates the total amount of shares that users have requested to withdraw and then loops through the list of users that have requested a withdrawal. For each user, it calculates the amount of shares that the user has requested to withdraw and then adds this amount to the withdrawManager.

If a malicious user was to perform a large number of requests to withdraw shares, the endWeeklySettle function would have to loop through all of these requests, which could lead to a DOS attack, leaving the contract unusable.

Impact

The contract might become unusable, temporarily locking up funds.

Recommendations

The team is aware of this issue and has partly addressed it by limiting the number of withdrawal requests that can be made, through the maxWithdrawCount variable.

We recommend taking appropriate measures to prevent this DOS attack by constantly monitoring the gas consumption and limitations of the chain that the contract is deployed on.

Remediation

This issue has been acknowledged by WOOFI, and a fix was implemented in commit da6cf56f.

A new function batchEndWeeklySettle was implemented in order to split the weekly settlement into batches. Our assumption is that this function will mainly be used in situations where endWeeklySettle will not work.

Zellic © 2024Back to top ↑