hats-finance / Accumulated-finance-0x75278bcc0fa7c9e3af98654bce195eaf3bb6a784

0 stars 0 forks source link

unbonded `withdrawalIds` length in `processWithdrawals()` function can result in DOS #39

Open hats-bug-reporter[bot] opened 2 weeks ago

hats-bug-reporter[bot] commented 2 weeks ago

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

Description: Description\ BaseMinterWithdrawal.sol provides ERC721 withdrawal requests. The owner of contract can set minWithdrawalAmount which must be met in order to call requestWithdrawal() function by users. The withdrawal request is later queued and processed by owner via processWithdrawals() function:

    function processWithdrawals(uint256[] calldata withdrawalIds) public onlyOwner {
        uint256 totalWithdrawals;
@>        for (uint256 i = 0; i < withdrawalIds.length; i++) {
            uint256 withdrawalId = withdrawalIds[i];
            WithdrawalRequest storage request = _withdrawalRequests[withdrawalId];
            require(request.amount > 0, "ZeroAmount");
            require(!request.processed, "AlreadyProcessed");
            require(!request.claimed, "AlreadyClaimed");

            request.processed = true;
            totalWithdrawals = totalWithdrawals+request.amount;

            emit ProcessWithdrawal(withdrawalId);
        }
        require(totalWithdrawals <= totalPendingWithdrawals, "TotalWithrawalAmountExceeded");
        stakingToken.burn(totalWithdrawals);
        totalPendingWithdrawals = totalPendingWithdrawals-totalWithdrawals;
        totalUnclaimedWithdrawals = totalUnclaimedWithdrawals+totalWithdrawals;
    }

withdrawalIds is passed as an dynamic array argument which does not have fixed length to process users's withdrawals.

This issue can result in vulnerabilities like a function within a contract iterates through an array or list i.e withdrawalIds. If this array becomes excessively large, iterating over it could consume gas past the block limit.

If this results in out of Gas vulnerability then the impact would be:

1) Denial of Service (DoS): processWithdrawals() function affected by this vulnerability can be rendered unusable, causing service disruptions.

2) Locked Funds: There could be temporarary locked funds due to failure of withdraw request processing.

OASIS sapphire has block gas limit of 15 million which is less than Ethereum's 30M block gas limit. If the minimum withdrawal amount is 1 or 5, the malicious user can create high number of withdrawal requests as number of withdrawalIds as NFT would be minted to malicious users. When such high number of withdrawal requests are queued and will be processed by owner then it will result in revert and denial of service as the function would have crossed OASIS 15 million gas limit. This revert would make the function fail.

Recommendations\ Consider having upper bound check on number of withdrawalIds in processWithdrawals() to allow them to be processed at a time.

ilzheev commented 2 weeks ago

When such high number of withdrawal requests are queued and will be processed by owner then it will result in revert and denial of service as the function would have crossed OASIS 15 million gas limit. This revert would make the function fail.

  1. Design allows to process withdrawals by limiting the number of withdrawals specifically for this case
  2. Math is incorrect. Here is the example of recent 100+ withdrawals processed and less than 2M gas spent: https://zetachain.blockscout.com/tx/0xa46f9ad159f29c0441f57346a424e8a4a052ddc9d2d6c186680656f40b475c6d