Improper handling of deposit fees leads to unintended asset extraction and may result in protocol insolvency

[hats-bug-reporter[bot]]

Github username: @MatinR1 Twitter username: MatinRezaii1 Submission hash (on-chain): 0xd9f2d633647659b18b4f3593fb29e1ead5a69ed1b9611969de1c0ab20022c352 Severity: high

Description: Description\ There is a discrepancy in how fees are managed within the system, specifically between withdrawal and deposit fees:

• Withdrawal Fees: These fees have a clearly defined path for collection and processing, ensuring that the user's assets are not adversely affected when the fees are collected. This approach effectively isolates the fee handling from the user's core assets.

• Deposit Fees: In contrast, deposit fees lack a specific path for collection. As a result, there is no defined mechanism for handling these fees, potentially leading to confusion or early and unprogrammed asset extraction in the deposit process. The absence of a dedicated collection process for deposit fees can result in these fees being either overlooked or improperly managed, which could impact the user's assets and the system's overall integrity.

If we look at the NativeMinterWithdrawal's (the one which is inherited by the stROSEMinter) deposit function, we can see how the asset is collected:

    function deposit(address receiver) public payable virtual nonReentrant {
        require(msg.value > 0, "ZeroDeposit");
        uint256 mintAmount = previewDeposit(msg.value);
        require(mintAmount > 0, "ZeroMintAmount");
        stakingToken.mint(receiver, mintAmount);
        emit Deposit(address(msg.sender), receiver, msg.value);
    }

    function previewDeposit(uint256 amount) public view virtual returns (uint256) {
        uint256 feeAmount = amount*depositFee/FEE_DENOMINATOR;
        uint256 netAmount = amount-feeAmount;
        return netAmount;
    }

As it is clear from above, the fee amount is subtracted from the initial deposit amount (msg.value for the native version), and the rest is minted for the user. The problem lies in the fee distribution mechanism. The deposit fees lack a separate collection function and recipient, which makes the fee-splitting hard. As opposed to the withdrawal process, which clearly has a distinct stream together with its collection function (with the option of choosing a recipient), the deposit fees lack such a mechanism.

Also, the overrided withdraw() function, which is the only way of extracting deposit fees, just checks for a positive balance of the contract after subtracting the unclaimed withdrawals and transfers the ETH to the desired receiver. By doing so, the recently deposited assets, without extracting the fee stream, are transferred to the receiver address by the owner. This leads to loss of assets and balance drainage of the contract which causes protocol insolvency, in the case of the recipient address couldn't provide the assets back in the next withdrawal batch.

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

        if (availableBalance < totalUnclaimedWithdrawals) {
            balance = 0;
        } else {
            balance = availableBalance-totalUnclaimedWithdrawals;
        }

        return balance;
    }

    function withdraw(address receiver) public virtual onlyOwner override {
        uint256 balance = balanceAvailable();
        require(balance > 0, "BalanceNotEnough");
        SafeTransferLib.safeTransferETH(receiver, balance);
        emit Withdraw(address(msg.sender), receiver, balance);
    }

Impact\ The deposit fees stream together with their collection mechanism is not defined and in the case, if the owner wants to extract it, this act may lead to unintended balance extraction of the recently deposited assets and even a drastic reduction of protocol balance which may lead to insolvency.

The impacts to be simplified:

1. Improper tracking of deposit fees
2. Temporary (or maybe permanently) loss of assets
3. Protocol insolvency

Attack Scenario\ A possible scenario would be:

1. The deposit fees equal to 5e18 are accumulated in the contract Minter.
2. The owner decides to collect these fees by calling the withdraw().
3. Recently staked amounts that are not requested for a withdrawal are equal to 55e18.
4. The owner sends the total amount of 60e18 to the receiver.
5. The depositors want to withdraw their assets and submit a withdrawal request
6. The receiver is unable to pay back the initially received assets
7. Thus, the withdrawal process will not happen due to insufficient balance.

Attachments

2. Revised Code File (Optional)

Consider adding a collection mechanism for deposit fees as it is for withdrawal fees. This requires the deposit fees being separated from the initial deposit amount. I mean adding something like this:

+   uint totalDepositFees;
+   event CollectDepositFees(address sender, address receiver, uint totalFees);

    function deposit(address receiver) public payable virtual nonReentrant {
        require(msg.value > 0, "ZeroDeposit");
        uint256 mintAmount = previewDeposit(msg.value);
        require(mintAmount > 0, "ZeroMintAmount");
        stakingToken.mint(receiver, mintAmount);
        emit Deposit(address(msg.sender), receiver, msg.value);
    }

    function previewDeposit(uint256 amount) public view virtual returns (uint256) {
        uint256 feeAmount = amount*depositFee/FEE_DENOMINATOR;
        uint256 netAmount = amount-feeAmount;
+       totalDepositFees += feeAmount
        return netAmount;
    }

+   function collectDepositFees(address receiver) public onlyOwner {
+       require(totalDepositFees > 0, "ZeroFees");
+       SafeTransferLib.safeTransferETH(receiver, totalDepositFees);
+       totalDepositFees = 0;
+       emit CollectDepositFees(address(msg.sender), receiver, totalDepositFees);
+   }

[0xRizwan]

Duplicate of #9