Loss of deposit fee to protocol

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x017dffd99902155af2700a14f8e27b74075509538389699da72d08cc2cd608cb Severity: high

Description: Description\ Protocol is expected to charge the deposit, withdraw and redeem fee which is capped as maximum depending on fee. 500 BPS would be maximum fee which can be charged in case of Deposits, Withdrawals and Redeem of tokens.

The issue is about failure in withdrawing the deposit fee to protocol's receiver address i.e loss of deposit fee which is also loss of additional revenue.

stROSEMinter.sol is the main contract which inherits NativeMinterWithdrawal from Minter.sol contract and further inheritance as follows:

contract stROSEMinter is NativeMinterWithdrawal {

NativeMinterWithdrawal.sol inherits `

contract NativeMinterWithdrawal is BaseMinterWithdrawal, NativeMinter {

NativeMinterWithdrawal.sol inherits NativeMinter contract which has deposit and withdraw functions and these are implemented as:

    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 withdraw(address receiver) public virtual onlyOwner {
        uint256 availableBalance = address(this).balance;
        require(availableBalance > 0, "ZeroWithdraw");
        SafeTransferLib.safeTransferETH(receiver, availableBalance);
        emit Withdraw(address(msg.sender), receiver, availableBalance);
    }

Below is the expected functionality of deposit():

1) The user deposits native ROSE token by calling deposit() function. To be noted, this would be directly accessible at stROSEMinter contract due to inheritance of contracts.

2) Contract will deduct fee by calculating via previewDeposit() function. The amount - fee will be mintAmount and this mintAmount will be minted as stROSE token to receiver address.

Below is the expected functionality of withdraw():

1) Whenever the deposit fee is paid by users to contract then owner is expected to withdraw the fee by calling withdraw() function. This action by owner will transfer all native ROSE token to owner's receiver address i.e all deposit fee would be transferred.

NOW, the issue is since stROSEMinter.sol follows multiple contracts inheritance which is detailed above, the withdraw() function will always revert. This is due to withdraw() is overridden in stROSEMinter contract which is implemented as below:

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

Therefore, any deposit fee paid by users while depositing ROSE token wont be withdrawable to owner's receiver address. This is due to lack of deposit fee withdrawal functionality.

It should be noted that, deposit fee is not tracked which is different issues and it can be mitigated with this issue.

Impact\ Loss of revenue to protocol due to permanent lock of deposit fees. Therefore, high severity issue.

Recommendations\ Suggest to implement collectDepositFees() where totalDepositFee should also be tracked, similar to collectWithdrawalFees()

Consider implementing following changes in NativeMinter.sol contract:

+    uint256 public totalDepositFees = 0;      // total fees accumulated

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

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

This function would be accessible at stROSEMinter due to inheritance with NativeMinter contract.

[ilzheev]

It's intended design. Protocol or team does not collect deposit fees, they are naturally deducted if necessary (e.g. bridging fees).

For most LST minters deposit fees are 0. If it's sidechain LST deployment (e.g. https://accumulated.finance/stake/vlx/#bnb), tokens are automatically bridged to the destination chain and bridge fee is applied:

    function deposit(uint256 amount, address receiver) public override nonReentrant {
        require(amount > 0, "ZeroDeposit");
        uint256 mintAmount = previewDeposit(amount);
        require(mintAmount > 0, "ZeroMintAmount");
        baseToken.safeTransferFrom(address(msg.sender), address(this), amount);
        baseTokenERC677.transferAndCall(address(bridge), amount, abi.encodePacked(destination));
        stakingToken.mint(receiver, mintAmount);
        emit Deposit(address(msg.sender), receiver, amount);
    }

In this example, user deposits 100 VLX, receives 99.9 stVLX. 100 VLX is sent to the bridge, and 99.9 VLX received on the destination chain and then staked.