hats-finance / Accumulated-finance-0x75278bcc0fa7c9e3af98654bce195eaf3bb6a784

0 stars 0 forks source link

`redemption fee` is not tracked similar to withdrawal fee #15

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

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

Github username: -- Twitter username: -- Submission hash (on-chain): 0x4171fdf94cbd0a8681863d5315b69578ffb7ee7e6ce7c980bc609d86059bcf3a Severity: low

Description: Description\ NativeMinterRedeem.sol is contract which is used by users to redeem their tokens. StakingToken stROSE would be burn and the ROSE is sent to user desired receipient address.

    function redeem(uint256 amount, address receiver) public virtual nonReentrant {
        require(amount > 0, "ZeroRedeem");
        uint256 redeemAmount = previewRedeem(amount);
        require(redeemAmount > 0, "ZeroRedeemAmount");
        stakingToken.safeTransferFrom(address(msg.sender), address(this), amount);
        stakingToken.burn(amount);
        SafeTransferLib.safeTransferETH(receiver, redeemAmount);
        emit Redeem(address(msg.sender), receiver, amount);
    }

The redemption fee is implemented in previewRedeem() function which substract fee and send the redeemAmount as ROSE to recipient address.

The issue is that, when the contract owner calls withdraw() from inherited NativeMinter contract, the totalRedeemFees is not tracked.

    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);
    }

It should be noted that, totalWithdrawalFees is tracked in case of withdrawals but in case of redeem. This is design descrepancy which must be fixed for redeem.

Recommendations\ Track the totalRedeemFees similar to totalWithdrawalFees in case of withdrawals.

Consider below changes in NativeMinterRedeem contract.

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

    function redeem(uint256 amount, address receiver) public virtual nonReentrant {
        require(amount > 0, "ZeroRedeem");
        uint256 redeemAmount = previewRedeem(amount);
        require(redeemAmount > 0, "ZeroRedeemAmount");
+      totalRedeemFees = totalRedeemFees + amount - redeemAmount;
        stakingToken.safeTransferFrom(address(msg.sender), address(this), amount);
        stakingToken.burn(amount);
        SafeTransferLib.safeTransferETH(receiver, redeemAmount);
        emit Redeem(address(msg.sender), receiver, amount);
    }

    function withdraw(address receiver) public virtual onlyOwner {
-        uint256 availableBalance = address(this).balance;
-        require(availableBalance > 0, "ZeroWithdraw");
+        require(totalRedeemFees > 0, "ZeroFees");
+        totalRedeemFees = 0;
+        SafeTransferLib.safeTransferETH(receiver, totalRedeemFees );
+        emit Withdraw(address(msg.sender), receiver, totalRedeemFees );
-        SafeTransferLib.safeTransferETH(receiver, availableBalance);
-        emit Withdraw(address(msg.sender), receiver, availableBalance);
    }
ilzheev commented 2 months ago

Similar to this: https://github.com/hats-finance/Accumulated-finance-0x75278bcc0fa7c9e3af98654bce195eaf3bb6a784/issues/9#issuecomment-2326031829

It's intended design. Protocol or team does not collect redeem fees. Redeem minter allows users to instantly redeem baseToken and does not require processing by multisig admin.

In real world it's used in liquid restaking (e.g. https://accumulated.finance/stake/stmtrg), where base token is rebase LST and multisig admin does not have to do anything with user deposits. Redeem fee, if applied, creates additional revenue to other users that keep and stake LRT.

0xRizwan commented 2 months ago

ok, how owner accessed withdraw() function would be used in NativeMinterRedeem.sol. If redeem fee wont be withdrawan then withdraw() function use is obslate.