Possible DOS
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.