User can inflate the totalUnclaimedWithdrawals amount and therefore DOS the withdrawal system

[hats-bug-reporter]

Github username: --
Twitter username: --
Submission hash (on-chain): 0xc364a37804a3b7b6c8e8d7793763a89cdd0ae40fad6ad0d86bd4be6946b58f6f
Severity: medium

Description:

Description

Function processWithdrawals processes the withdrawals, this function is called by the onlyOwner, at the end the amounts are added to the totalUnclaimedWithdrawals:

    function processWithdrawals(uint256[] calldata withdrawalIds) public onlyOwner {

=>        totalUnclaimedWithdrawals = totalUnclaimedWithdrawals+totalWithdrawals;
    }

Now whenever the user wants to claim his withdrawal he can do so by calling claimWithdrawal, note that this function can only be called by the owner of the withdrawalId:

    function claimWithdrawal(uint256 withdrawalId, address receiver) public virtual nonReentrant {
=>        require(ownerOf(withdrawalId) == msg.sender, "NotOwner");
        WithdrawalRequest storage request = _withdrawalRequests[withdrawalId];
        require(request.processed, "NotProcessedYet");
        _burn(withdrawalId);
        request.claimed = true;
        totalUnclaimedWithdrawals = totalUnclaimedWithdrawals-request.amount;
        SafeTransferLib.safeTransferETH(receiver, request.amount);
        emit ClaimWithdrawal(address(msg.sender), receiver, request.amount, withdrawalId);
    }

At the end of the flow ,amount is subtracted from totalUnclaimedWithdrawals.

The problem however is that the user can do the following:

• Bob calls requestWithdrawal with a large amount
• processWithdrawals is invoked and the amount is added to totalUnclaimedWithdrawals
• Bob will refrain from calling claimWithdrawals resulting in totalUnclaimedWithdrawals staying inflated with Bob's amount
• Because of this whenever a multi sig wants to move tokens to stake them by calling the withdraw function, it will invoke balanceAvailable which will perform the following check:

    function balanceAvailable() public view virtual returns (uint256) {
        uint256 availableBalance = address(this).balance;
        uint256 balance;

=>        if (availableBalance < totalUnclaimedWithdrawals) {
            balance = 0;

• Because Bob keeps the totalUnclaimedWithdrawals inflated with a high request.amount the onlyOwner will be unable to make use of the withdraw function
• Note that there is no way for the onlyOwner to call claimWithdrawal for a user, because of this no one is able to intervene with Bob not claiming

This essentially breaks a core functionality which is that the onlyOwner is unable to perform withdraw due to a malicious user refraining from claiming

Recommendation

this can easily be fixed by introducing a function that allows the onlyOwner to forcefully claim for a user if the user themselves refrain from doing so or even if they just forget to do so.

[0xRizwan]

The claiming of withdrawals completely lies on users whenever they want to claim their tokens back. The issue described is intended behaviour of protocol as the owner is expected to withdraw the tokens if they are available. In context of this competition scope, withdrawals are disabled in stROSEMinter.sol contract where withdraw() is overridden from inherited NativeMinterWithdrawal contract.

    function withdraw(address /* receiver */) public view onlyOwner override {
        revert(string(abi.encodePacked("Withdrawals disabled")));
    }

Therefore, i believe its non-issue.

[whoismxuse]

Hi @0xRizwan,

The claiming of withdrawals completely lies on users whenever they want to claim their tokens back

Thats exactly the issue, users can inflate the totalUnclaimedWithdrawals since they can decide whether to claim or not.

In context of this competition scope, withdrawals are disabled in stROSEMinter.sol contract where withdraw() is overridden from inherited NativeMinterWithdrawal contract.

I asked the sponsor regarding the functionality of this withdraw function and was told this is used if a multi-sig wants to stake tokens, so this function definetely gets used

This issue should be valid

[ilzheev]

@0xRizwan described it correctly. Multisig owner processes withdrawals based on the current minter balance – if there are not enough tokens to cover withdrawal request, it's not processed.

@fonstack totalUnclaimedWithdrawals is increased only by multisig owner in processWithdrawals(uint256[] calldata withdrawalIds) public onlyOwner, so user cannot inflate this amount.

[whoismxuse]

Hi @ilzheev Thank you for commenting,

Multisig owner processes withdrawals based on the current minter balance – if there are not enough tokens to cover withdrawal request, it's not processed

I never disagreed with this, in fact, I agree that if there are not enough tokens it will not be processed.

totalUnclaimedWithdrawals is increased only by multisig owner in processWithdrawals(uint256[] calldata withdrawalIds) public onlyOwner, so user cannot inflate this amount.

I think you have misunderstood the issue here, my issue talks about the multisig owner calling processWithdrawals on a large request.amount which adds the amount to the totalUnclaimedWithdrawals but due to the user not calling claimWithdrawal he will keep the totalUnclaimedWithdrawals inflated with that amount, since the amount only gets removed upon calling claimWithdrawal

        totalUnclaimedWithdrawals = totalUnclaimedWithdrawals-request.amount;

Which only the user can call and he will refrain from calling.

Because of this multisig withdrawals will revert since everytime they call this function they will not have enough tokens to cover the withdrawal.

[ilzheev]

@whoismxuse here is an example of token flow.

1. Users deposited 1000 X into Minter, no one requested withdrawals
   --
   Minter balance: 1000 X + 0 stX

2. User requested to withdrawal id=0 of 100 X by calling requestWithdrawal()
   --
   Minter balance: 1000 X + 100 stX
totalPendingWithdrawals: 100 (assuming fees are 0)
   totalUnclaimedWithdrawals: 0 (no withdrawals were processed yet)
   balanceAvailable: 1000

3. Multisig owner processed withdrawal id=0 by calling processWithdrawals()
   --
   Minter balance: 1000 X + 0 stX (stX is burnt in processWithdrawals())
   totalPendingWithdrawals: 0 (reduced by 100 in processWithdrawals())
   totalUnclaimedWithdrawals: 100 (increased by 100 in processWithdrawals())
   balanceAvailable: 900

Now, 900 X are available to be used by multisig admin, while 100 X are reserved for withdrawal id=0, so user will be able to claim them whenever he wants (even in 1 week, 1 month, 1 year, and so on).

There is an example how it works in production:

• There are 7.3K MANTA in minter (5.4K available + 1.9K reserved for already processed withdrawals)
• 5.7K MANTA pending withdrawals, that are not processed yet due to lack of available tokens in the minter

From documentation (to better understand how withdrawals work):

stMANTA direct redemptions typically take between 1 and 7 days. The duration depends on the deposit buffer. If the buffer has enough tokens, the process is quicker. If not, unstaking tokens can take up to 7 days, ensuring network security and proper liquidity management.

Thus multi-sig owner will unstake and add additional tokens into minter to process pending withdrawals, or deposit buffer held in the minter contract will be increased by new user deposits.

[whoismxuse]

Hi @ilzheev ! Thanks for the elaborate example, I truly appreciate it.

The issue however, still stands.

If the multisig owner processes a whale's withdrawal request.amount (let's assume it to be as big as the current available supply), the whale will just refrain from calling claimWithdrawal and thereby DOS'ing the balanceAvailable system. No one can do anything about this.

The whale can repeat this everytime more funds become available and eventually can hold the total available MANTA.

Note that this is realistic since:

5.4K available

This reflects to roughly 3500$ as of now.

a whale can easily absorb all these tokens and refrain from claiming them. This is possible since there is no force claim function that allows the multsig to claim for a user, for example, in these particular situations.

[0xRizwan]

@whoismxuse As i mentioned, withdrawals are disabled in case of inscope stROSEMinter contract. The scenario, you have mentioned does not have any monetary gain to malicious user. Sponsor already clarified and imo, decision should be respected.